Repository navigation
Conversation
modifyLiquidities opens its own PoolManager unlock, so when a route runs inside an unlock someone else opened the call can only fail with AlreadyUnlocked, and FLAG_ALLOW_REVERT would turn that into a silent skip of a leg whose funds are already parked in the position manager. Gate the command on isUnlocked in the router's own frame, where the flag does not reach, and let the existing sub-plan re-throw carry the error out.
…able PERMIT2_TRANSFER_FROM_BATCH passed each detail's `to` straight to Permit2, so the MSG_SENDER and ADDRESS_THIS sentinels every other recipient field honours sent tokens to address(1) and address(2) instead. Permit2Payments cannot reach _mapRecipient, so move the per-detail loop into the Dispatcher branch beside the single-transfer command and map there.
A command built on an external call fails with no returndata when its callee runs out of gas, and FLAG_ALLOW_REVERT tolerated that like any other failure even though the transaction's gas limit is the submitter's choice. Snapshot gasleft before dispatch and, on failure, revert with InsufficientGas when the frame holds under 1/64 of it, the share EIP-150 leaves behind a callee that ran dry. The check runs before the required-command check: after it, a starved required command inside an allow-revert sub-plan would surface as a plain ExecutionFailed that the parent swallows. EXECUTE_SUB_PLAN re-throws InsufficientGas the same way it re-throws the nested-execution gate.
The recipient's balance was snapshotted before the first hop, so when the recipient is itself a pair the path passes through, the tokens an earlier hop routed into it counted toward amountOutMinimum along with the final hop's output. Take the snapshot right before the final hop instead, so a route whose output token reappears mid-path cannot satisfy its minimum with its own intermediate transfer.
The gas-starvation check adds a gasleft read and a comparison per command, the batch transfer loop moved into the Dispatcher, and the v2 delivery snapshot moved inside the hop loop. Regenerated with forge v1.4.3 on the path, the version CI pins, since forge 1.8.3 emits context remappings that hardhat-foundry 1.1.2 cannot parse.
🤖 Claude PR Metadata GenerationStatus: ❌ Error
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for four of the eight contest findings filed against
dev-2.3.0at26f89e6, one commit each with a regression test that fails on the unfixed code, plus README text for the new behavior and regenerated Hardhat gas snapshots. The OZ findings were already resolved on the base. The other four findings are covered at the end: three acknowledged without code changes, one a comment fix in v4-periphery.What's in it
V4_POSITION_MANAGER_CALLrefuses to run inside a foreign PoolManager unlock. The command only admitsmodifyLiquidities, which opens its own unlock, so inside an unlock someone else opened it could only fail withAlreadyUnlocked. That failure fell underFLAG_ALLOW_REVERT, so a plan built with the flag completed with the leg skipped and the route's funds left in the position manager: the L-01 shape, which the 2.3.0 gate closed forV4_SWAPbut not here. The branch now readspoolManager.isUnlocked()after the calldata check and revertsNestedExecutionNotPermittedin the router's own frame, where the flag does not reach. The existingEXECUTE_SUB_PLANre-throw carries it out of nested plans.PERMIT2_TRANSFER_FROM_BATCHmaps each detail'stothrough_mapRecipient. The batch passed recipients straight to Permit2, so theMSG_SENDERandADDRESS_THISsentinels every other recipient field honors sent tokens to address 1 and address 2 instead.Permit2Paymentssits below the contract that defines the mapper, so the per-detail loop moves into the Dispatcher branch beside the single-transfer command, and the batch overload inPermit2Paymentsis removed with its imports.FLAG_ALLOW_REVERT. A call-based command fails with no returndata when its callee runs out of gas, and the flag tolerated that like any other failure, even though the gas limit is the one input a route's submitter controls. The execute loop snapshotsgasleft()before dispatch and, on failure, revertsInsufficientGaswhen the frame holds under 1/64 of it, the share EIP-150 leaves behind a callee that ran dry (the check OpenZeppelin's forwarder uses). It runs before the required-command check on purpose: after it, a starved required command inside an allow-revert sub-plan would surface as a plainExecutionFailedthat the parent frame, still holding about 2/64 of its own gas, would swallow.EXECUTE_SUB_PLANre-throwsInsufficientGasthe same way it re-throws the nested-execution gate. Pre-existing since v1._v2Swapmeasures delivery across the final hop only. The recipient's balance was snapshotted before the first hop, so when the recipient is itself a pair the path passes through, the tokens an earlier hop routed into it counted towardamountOutMinimum. The snapshot now happens inside the loop at the penultimate pair, right before the final swap; the check after the loop is unchanged. Pre-existing since v1.The README documents the failures the flag never swallows, the batch recipient sentinels, and the final-hop measurement.
Alternatives considered
AlreadyUnlockedselector and re-throwing costs more bytes and ties the router to the position manager's error. Making the command ignore the flag altogether breaks plans that legitimately tolerate a failed mint at top level. Gating on the nested opt-in instead of the unlock state is wrong, since an opted-in route still cannot run it. The gate sits after the calldata check so a malformed input still reports the slice error first, and a chain without a pool manager never reads one before validating the input.invalid()saves 12 bytes and burns the submitter's remaining gas, which is what OpenZeppelin does, but gives integrators nothing to decode. Checking only on the tolerated path leaves the sub-plan hole above. A per-command gas stipend or a minimum-gas field in the plan changes the format and the SDK.Not in this branch
SafeCastOverflow. Acknowledged: core bounds each operation toint128, the amounts exceed any supply, and a plan can split.PositionInfoLibraryfield list namingtickUpperbeforetickLowerwhile claiming LSB order. Fixed in v4-periphery as a comment-only commit, a14b2e80 onfix/position-info-layout-commentoff peripherymain. The file is identical on the7ed84398pin, so the submodule pointer here is unchanged.Bytecode
UniversalRouterruntime, fromsnapshots/UniversalRouterTest.jsonat each commit:dev-2.3.0(26f89e6)Gas
66 Hardhat snapshot entries changed, median 13 gas lower. PositionManager mint paths pay about 1.0k to 1.3k more for the
isUnlockedread; long multi-swap plans are down by as much as 1.7k.Testing
New Foundry tests, each run against the unfixed code first and failing there:
V4PositionManagerCallWithinUnlock.t.sol: deploys a realPositionManagerfrom its artifact, since importing it clashes with the per-fileoptimizer_runsrestrictions infoundry.toml. A top-level mint succeeds as a control; inside a foreign unlock the command reverts with the flag clear, with the flag set, and with the flag set on an enclosing sub-plan.Permit2TransferFromBatch.t.sol:ADDRESS_THISdelivers to the router,MSG_SENDERto the caller, and a detail whosefromis not the caller revertsFromAddressIsNotOwner.GasStarvation.t.sol: aRESOLVEagainst a resolver that burns all forwarded gas, under a 1M gas limit, revertsInsufficientGasas a required command, as an allow-revert command, and as either inside an allow-revert sub-plan. Two controls show ordinary failures still follow the flag.V2RecipientPairAlias.t.sol: fork test at block 20010000 against the real v2 factory with four mock tokens, path A, B, C, D, B, delivering to the B-C pair. A minimum one wei above the final hop's output revertsV2TooLittleReceived; the exact final-hop output passes; an ordinary recipient receives the final hop's output.